Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DIP-273 Content Addressing #280

Merged
merged 12 commits into from
Aug 13, 2024
Merged

Conversation

wesbiggs
Copy link
Member

@wesbiggs wesbiggs commented Jul 1, 2024

Problem

DSNP should provide affordances for finding content that do not rely on a single DNS-based point of failure for content hosting.
More in the original discussion: #273

Solution

Enhance the specification to treat URLs in Announcements as suggestions rather than canonical locations for content.
Provide a simple and well-specified set of hashes and encodings that can be used consistently throughout the protocol.
Use IPFS CIDv1 specifically for locating profiles.

Change summary:

  • Broaden ProfileResource definition so that different types of profile resources can use different, possibly distributed, file systems via a generic contentAddress field
  • Simplify multihashes to use base32 encoding only and sha2-256 or blake3 as the hashing algorithm
  • Update various example hashes in line with this
  • Update to pre-1.3.0 versioning and sync prerelease changelogs for other recent additions to the spec
  • Change spec language to expand on how to treat content hash + URL pairs.
  • Announcements use the same base32 encoded multihash for the contentHash and targetContentHash fields.

* Update the definition of DSNP Content Hash to include DSNP CIDs
* Change ProfileResource to use bytes, not string, for cid field, in line with Announcement usage
* Update Activity Content Hash extension to include CID option
* Update various example hashes to use CIDv1
* Update to pre-1.3.0 versioning and sync prerelease changelogs
@wesbiggs wesbiggs force-pushed the proposal/DIP-273_Content_Addressing branch from 9332b55 to c571222 Compare July 1, 2024 20:41
@wesbiggs wesbiggs requested a review from harry-evans July 1, 2024 20:59
@@ -69,9 +70,9 @@ In order for DSNP applications to interoperate, the required functionality is li

- CID version: MUST be version 1, in order to distinguish CIDs from simple multihash values in situations where either may be used
- Hash algorithm: MUST be `sha2-256` or `blake2b-256`
- Encoding: MUST be `base58btc` or `base32`
- Codec: MUST be `dag-pb` for data 256*1024 bytes or longer; `raw` for data less than 256*1024 bytes
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding backslash before * helps markdown to disambiguate:

  • Codec: MUST be dag-pb for data 256*1024 bytes or longer; raw for data less than 256*1024 bytes

Copy link
Member

@wilwade wilwade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good with the exception of some of the example hashes.


```json
{
"hash": [
"QmQNHNfHnbgJJ6nK4UPx2VtTUCafAKCbqZJ6ZRYUGjoeFj",
"2DrjgbGgSsXRhTiBWckoVwBFC6H4qiBWWNumSsRwdUt82YnTdN"
"bciqgb7tvyyqn6tw4amtdj7funn5ockup6ldrkap4o5ubd77od6awi4y",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting a different value for the hash of the dsnp_whitepaper than this is giving me.

  1. Perhaps it is the older version of the whitepaper the hash is of?
  2. Different hash setup that is reading the file differently than I did?
    • I tried sha256sum on mac and a random online hasher.

The digest I expected from hashing the file was 0x36d37cf984565f2151ccea050e0bcecf44801b4c89971654c7308dc2338ce7ea

However the digest from this multihash appears to be 0x60fe75c620df4edc032634fcb46b7ae12a8ff2c71501fc776811ffee1f816473

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, my mistake. Will fix.

"QmQNHNfHnbgJJ6nK4UPx2VtTUCafAKCbqZJ6ZRYUGjoeFj",
"2DrjgbGgSsXRhTiBWckoVwBFC6H4qiBWWNumSsRwdUt82YnTdN"
"bciqgb7tvyyqn6tw4amtdj7funn5ockup6ldrkap4o5ubd77od6awi4y",
"bdyqdua4t4pxgy37mdmjyqv3dejp5betyqsznimpneyujsur23yubzna"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for this one, so I think it might just be we are using different files.

Copy link
Member

@wilwade wilwade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@wesbiggs wesbiggs enabled auto-merge (squash) August 12, 2024 20:11
Copy link
Collaborator

@shannonwells shannonwells left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read through the referenced Issue thread, read through the changes, looks ok by me.

| emoji | `BYTE_ARRAY` | `STRING` | `UTF8` |
| fromId | `INT64` | `INT(64, false)` | `UINT_64` |
| inReplyTo | `BYTE_ARRAY` | `STRING` | `UTF8` |
| targetContentHash | `BYTE_ARRAY` | `STRING` | `UTF8` |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: Confirming on gateway side this is already updated

Copy link
Collaborator

@shannonwells shannonwells left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approved

Copy link
Collaborator

@shannonwells shannonwells left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This time for sure

@wesbiggs wesbiggs merged commit 0a81372 into main Aug 13, 2024
1 check passed
@wesbiggs wesbiggs deleted the proposal/DIP-273_Content_Addressing branch August 13, 2024 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants